Skip to content

Conversation

kessplas
Copy link
Contributor

@kessplas kessplas commented Aug 14, 2025

Issue #, if available:

Description of changes:

Spec for S3EC Keyring/S3 Keyring/KmsKeyring.
Java annotations: aws/amazon-s3-encryption-client-java#483

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

@kessplas kessplas requested a review from a team as a code owner August 14, 2025 21:37
Copy link
Contributor

@josecorella josecorella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nits and questions

The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL"
in this document are to be interpreted as described in [RFC2119](https://tools.ietf.org/html/rfc2119).

## Interface
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe too much for this review, but similarly to the mpl keyring interface,
https://github.com/awslabs/aws-encryption-sdk-specification/blob/master/framework/keyring-interface.md#interface
you may want to include more generic information on how the encrypt and decrypt operations should do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add a short non-RFC 2119 sentence for each.


## Interface

The KmsKeyring MUST implement the [Keyring interface](keyring-interface.md#interface) and include the behavior described in the [S3 Keyring](s3-keyring.md).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you mean,

Suggested change
The KmsKeyring MUST implement the [Keyring interface](keyring-interface.md#interface) and include the behavior described in the [S3 Keyring](s3-keyring.md).
The KmsKeyring MUST implement the [Keyring interface](keyrings.md#interface) and include the behavior described in the [S3 Keyring](s3-keyring.md).

?

The KmsKeyring MAY validate that the AWS KMS key identifier is not null or empty.
If the KmsKeyring validates that the AWS KMS key identifier is not null or empty, then it MUST throw an exception.
The KmsKeyring MAY validate that the AWS KMS key identifier is [a valid AWS KMS Key identifier](../../framework/aws-kms/aws-kms-key-arn.md#a-valid-aws-kms-identifier).
If the KmsKeyring validates that the AWS KMS key identifier is not a valid AWS KMS Key identifier, then it MUST throw an exception.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: perhaps a good start to specify the type of exception that gets thrown or at least the error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally but since the S3EC V3 Java doesn't actually do these validations, it's currently unspecified.

Copy link
Contributor

@josecorella josecorella left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kessplas kessplas merged commit 280a894 into master Aug 25, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants